CLN GH22875 Replace bare excepts by explicit excepts in pandas/io/ #23004

JustinZhengBC · 2018-10-05T04:49:25Z

closes Replace bare excepts by explicit excepts in pandas/io/ #22875
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

This is a resubmit of a previous PR I submitted that was approved (#22916). I tried to close and reopen to get a rebuild on travis but I couldn't reopen it because I had deleted my old fork (after accidentally pushing to master).

pep8speaks · 2018-10-05T04:49:33Z

Hello @JustinZhengBC! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/io/clipboards.py !
There are no PEP8 issues in the file pandas/io/formats/console.py !
There are no PEP8 issues in the file pandas/io/formats/terminal.py !
There are no PEP8 issues in the file pandas/io/packers.py !
There are no PEP8 issues in the file pandas/io/parsers.py !
There are no PEP8 issues in the file pandas/io/pickle.py !
There are no PEP8 issues in the file pandas/io/sas/sas_xport.py !
There are no PEP8 issues in the file pandas/io/sas/sasreader.py !
There are no PEP8 issues in the file pandas/io/sql.py !
There are no PEP8 issues in the file pandas/io/stata.py !

Comment last updated on October 09, 2018 at 18:52 Hours UTC

jreback

looks good. small comment. ping on green.

jreback · 2018-10-05T12:04:23Z

pandas/io/pickle.py

    try:
        return try_read(path)
-    except:
+    except Exception:


can you add a comment here

codecov · 2018-10-05T14:48:36Z

Codecov Report

Merging #23004 into master will increase coverage by <.01%.
The diff coverage is 36.84%.

@@            Coverage Diff             @@
##           master   #23004      +/-   ##
==========================================
+ Coverage   92.19%   92.19%   +<.01%     
==========================================
  Files         169      169              
  Lines       50831    50911      +80     
==========================================
+ Hits        46864    46939      +75     
- Misses       3967     3972       +5

Flag	Coverage Δ
#multiple	`90.61% <36.84%> (ø)`	⬆️
#single	`42.3% <5.26%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pandas/io/clipboards.py	`100% <ø> (ø)`	⬆️
pandas/io/formats/terminal.py	`21.25% <0%> (ø)`	⬆️
pandas/io/sas/sas_xport.py	`90.23% <0%> (ø)`	⬆️
pandas/io/sas/sasreader.py	`86.2% <0%> (ø)`	⬆️
pandas/io/packers.py	`88.04% <50%> (ø)`	⬆️
pandas/io/parsers.py	`95.6% <50%> (ø)`	⬆️
pandas/io/formats/console.py	`75.75% <75%> (ø)`	⬆️
pandas/core/internals/concat.py	`98% <0%> (-0.38%)`	⬇️
pandas/core/indexes/period.py	`93.31% <0%> (-0.35%)`	⬇️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4976ee1...ee85901. Read the comment docs.

JustinZhengBC · 2018-10-05T18:49:54Z

@jreback the tests are green now.

Also, I have another PR with a similar problem (#22904). Is there a way to force travis to rebuild without committing new changes or should I just resubmit the PR?

jreback · 2018-10-06T15:06:03Z

Also, I have another PR with a similar problem (#22904). Is there a way to force travis to rebuild without committing new changes or should I just resubmit the PR?

you can just push another commit, or generate a new hash
git commit --all --amend -C HEAD

jreback · 2018-10-06T15:07:17Z

pandas/io/pickle.py

            # reg/patched pickle
-            try:
-                return read_wrapper(
-                    lambda f: pc.load(f, encoding=encoding, compat=False))


is there a reason you elminated this branch here? IIRC this was a perf issue (IOW try the compat=False), but I wrote this code a long time ago.

Yes, it appears that since then, someone has modified the load function so that compat isn't used at all.

pandas/pandas/compat/pickle_compat.py

Lines 189 to 214 in 5551bcf

def load(fh, encoding=None, compat=False, is_verbose=False):

"""load a pickle, with a provided encoding

if compat is True:

fake the old class hierarchy

if it works, then return the new type objects

Parameters

----------

fh: a filelike object

encoding: an optional encoding

compat: provide Series compatibility mode, boolean, default False

is_verbose: show exception output

"""

try:

fh.seek(0)

if encoding is not None:

up = Unpickler(fh, encoding=encoding)

else:

up = Unpickler(fh)

up.is_verbose = is_verbose

return up.load()

except (ValueError, TypeError):

raise

I am skeptical, I put this here because of performance issues. Can you run the asv for the pickle tests? though it might be only a case of reading py2 in py3 which is affected by this, which we prob don't perf check.

can you also see if we actually hit this branch in any tests (meaning the try part is certainly hit, but the except is the question)

jreback · 2018-10-07T21:51:09Z

pandas/io/pickle.py

            # reg/patched pickle
-            try:
-                return read_wrapper(
-                    lambda f: pc.load(f, encoding=encoding, compat=False))


I am skeptical, I put this here because of performance issues. Can you run the asv for the pickle tests? though it might be only a case of reading py2 in py3 which is affected by this, which we prob don't perf check.

jreback · 2018-10-07T21:51:44Z

pandas/io/pickle.py

            # reg/patched pickle
-            try:
-                return read_wrapper(
-                    lambda f: pc.load(f, encoding=encoding, compat=False))


can you also see if we actually hit this branch in any tests (meaning the try part is certainly hit, but the except is the question)

JustinZhengBC · 2018-10-08T01:10:11Z

Output of benchmark tests:

· Creating environments
· Discovering benchmarks
· Running 4 total benchmarks (2 commits * 1 environments * 2 benchmarks)
[  0.00%] · For pandas commit ee808033 <master> (round 1/2):
[  0.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt..............................................
[  0.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 12.50%] ··· Running (io.pickle.Pickle.time_read_pickle--)..
[ 25.00%] · For pandas commit 6b54cf8b <BUG-22984> (round 1/2):
[ 25.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[ 25.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 37.50%] ··· Running (io.pickle.Pickle.time_read_pickle--)..
[ 50.00%] · For pandas commit 6b54cf8b <BUG-22984> (round 2/2):
[ 50.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 62.50%] ··· io.pickle.Pickle.time_read_pickle                         23.0±2ms
[ 75.00%] ··· io.pickle.Pickle.time_write_pickle                        20.9±1ms
[ 75.00%] · For pandas commit ee808033 <master> (round 2/2):
[ 75.00%] ·· Building for conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt...
[ 75.00%] ·· Benchmarking conda-py3.6-Cython-matplotlib-numexpr-numpy-openpyxl-pytables-pytest-scipy-sqlalchemy-xlrd-xlsxwriter-xlwt
[ 87.50%] ··· io.pickle.Pickle.time_read_pickle                         21.9±1ms
[100.00%] ··· io.pickle.Pickle.time_write_pickle                        21.0±1ms

BENCHMARKS NOT SIGNIFICANTLY CHANGED.

Both the remaining except blocks are necessary, as removing them will result in test failures.

Upon further thought, I suppose it could be argued that the branch should remain in case someone modifies the load function later so that compat is used again. Should I put it back?

jreback · 2018-10-09T12:52:53Z

yeah let's just put it back, but you can add a TODO that should see if can be removed safely., breaking pickle is not great and this is not super well tested.

jreback · 2018-10-09T17:21:09Z

lgtm. merge on green.

datapythonista · 2018-10-10T07:33:53Z

Thanks for the fixes @JustinZhengBC, good work.

…ev#23004)

JustinZhengBC added 6 commits September 30, 2018 15:22

CLN GH22785 Replace bare excepts by explicit excepts in pandas/io/

6b9ebcf

CLN GH22875 Modify previous commit so flake8 passes

53d9fb8

CLN GH22875 Fix pandas/io/packers.py

f53b29f

CLN GH22875 Fix other except in pandas/io/packers.py

b4f15c7

CLN GH22875 Incorporated feedback from tests

8c17a95

CLN GH22875 Delete unnecessary comments and incorporate test feedback

90cfe17

jreback requested changes Oct 5, 2018

View reviewed changes

pandas/io/pickle.py Outdated

try:

return try_read(path)

except:

except Exception:

Copy link

Contributor

jreback Oct 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment here

jreback added the Error Reporting Incorrect or improved errors from pandas label Oct 5, 2018

jreback added this to the 0.24.0 milestone Oct 5, 2018

CLN GH22875 Add comments where necessry

b1d381a

jreback requested changes Oct 6, 2018

View reviewed changes

jreback requested changes Oct 7, 2018

View reviewed changes

CLN GH22875 Put back pickle branch and add TODO

e2e30bd

jreback approved these changes Oct 9, 2018

View reviewed changes

JustinZhengBC added 3 commits October 9, 2018 10:56

CLN GH22875 Retry travis build due to HTTP error

9ddfdb5

CLN GH22875 Fix linting problem

e6610fa

CLN GH22875 See if Travis works now

ee85901

datapythonista merged commit 1a61e26 into pandas-dev:master Oct 10, 2018

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

CLN: Replace bare excepts by explicit excepts in pandas/io/ (pandas-d…

0d66cc0

…ev#23004)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN GH22875 Replace bare excepts by explicit excepts in pandas/io/ #23004

CLN GH22875 Replace bare excepts by explicit excepts in pandas/io/ #23004

JustinZhengBC commented Oct 5, 2018 •

edited

Loading

pep8speaks commented Oct 5, 2018 •

edited

Loading

jreback left a comment

jreback Oct 5, 2018

codecov bot commented Oct 5, 2018 •

edited

Loading

JustinZhengBC commented Oct 5, 2018

jreback commented Oct 6, 2018

jreback Oct 6, 2018

JustinZhengBC Oct 6, 2018

jreback Oct 7, 2018

jreback Oct 7, 2018

jreback Oct 7, 2018

jreback Oct 7, 2018

JustinZhengBC commented Oct 8, 2018

jreback commented Oct 9, 2018

jreback commented Oct 9, 2018

datapythonista commented Oct 10, 2018

	def load(fh, encoding=None, compat=False, is_verbose=False):
	"""load a pickle, with a provided encoding

	if compat is True:
	fake the old class hierarchy
	if it works, then return the new type objects

	Parameters
	----------
	fh: a filelike object
	encoding: an optional encoding
	compat: provide Series compatibility mode, boolean, default False
	is_verbose: show exception output
	"""

	try:
	fh.seek(0)
	if encoding is not None:
	up = Unpickler(fh, encoding=encoding)
	else:
	up = Unpickler(fh)
	up.is_verbose = is_verbose

	return up.load()
	except (ValueError, TypeError):
	raise

CLN GH22875 Replace bare excepts by explicit excepts in pandas/io/ #23004

CLN GH22875 Replace bare excepts by explicit excepts in pandas/io/ #23004

Conversation

JustinZhengBC commented Oct 5, 2018 • edited Loading

pep8speaks commented Oct 5, 2018 • edited Loading

Comment last updated on October 09, 2018 at 18:52 Hours UTC

jreback left a comment

Choose a reason for hiding this comment

jreback Oct 5, 2018

Choose a reason for hiding this comment

codecov bot commented Oct 5, 2018 • edited Loading

Codecov Report

JustinZhengBC commented Oct 5, 2018

jreback commented Oct 6, 2018

jreback Oct 6, 2018

Choose a reason for hiding this comment

JustinZhengBC Oct 6, 2018

Choose a reason for hiding this comment

jreback Oct 7, 2018

Choose a reason for hiding this comment

jreback Oct 7, 2018

Choose a reason for hiding this comment

jreback Oct 7, 2018

Choose a reason for hiding this comment

jreback Oct 7, 2018

Choose a reason for hiding this comment

JustinZhengBC commented Oct 8, 2018

jreback commented Oct 9, 2018

jreback commented Oct 9, 2018

datapythonista commented Oct 10, 2018

JustinZhengBC commented Oct 5, 2018 •

edited

Loading

pep8speaks commented Oct 5, 2018 •

edited

Loading

codecov bot commented Oct 5, 2018 •

edited

Loading